fix: force serverless target #343

ascorbic · 2021-05-24T10:44:45Z

Uses an env var to emulate Vercel, then another to force the target to serverless if it's not set as serverless or experimental-serverless-trace

lindsaylevine · 2021-05-24T14:47:49Z

Fixes #204 :D

lindsaylevine

just need to update the tests :) plus few comments

lindsaylevine · 2021-05-25T05:19:41Z

helpers/hasCorrectNextConfig.js

+    delete require.cache[require.resolve('next/dist/next-server/server/config')]
+
+    // Clear memoized cache
+    getNextConfig.clear()


why do we have to clear here?

The function is memoized, so it will return the old value if we don't clear it

This works.
In the long run, we probably should consider removing memoization on getNextConfig() and passing its return value around function calls instead.

lindsaylevine · 2021-05-25T05:20:21Z

index.js

-      }
-    }
+    // Populates the correct config if needed
+    await doesNotNeedPlugin({ netlifyConfig, packageJson, failBuild })

    // Because we memoize nextConfig, we need to do this after the write file


we can prob remove this comment now i think?

double ping on this one?

Suggested change

// Because we memoize nextConfig, we need to do this after the write file

lindsaylevine · 2021-05-25T05:24:08Z

helpers/hasCorrectNextConfig.js

@@ -12,9 +11,27 @@ const hasCorrectNextConfig = async ({ nextConfigPath, failBuild }) => {
  const isValidTarget = acceptableTargets.includes(target)
  if (!isValidTarget) {
    console.log(
-      `Your next.config.js must set the "target" property to one of: ${acceptableTargets.join(', ')}. Update the
-      target property to allow this plugin to run.`,
+      `The "target" config property must be one of "${acceptableTargets.join('", "')}". Setting it to "serverless".`,


"Settingit to serverless" might imply to some that we're actually mutating/updating/changing their config file (which i don't think we're doing? 🤔 ). maybe "forcing the build to use target serverless" or something?

ascorbic · 2021-05-25T09:11:17Z

@ehmicky Can you think of a way for us to detect if the plugin is returning early when we're in tests? You mentioned here that we shouldn't return values in plugin methods.

ascorbic · 2021-05-25T12:40:25Z

@lindsaylevine what do you think about disabling those tests for now until we can work out a better way of checking if it's run?

lindsaylevine · 2021-05-25T12:52:27Z

@ascorbic we can def remove the next export one. how about setting an env variable at the end of each step and checking that? i dont see a problem w that 🤔

ehmicky · 2021-05-25T13:34:23Z

helpers/hasCorrectNextConfig.js

+    // https://github.com/vercel/next.js/blob/canary/packages/next/telemetry/ci-info.ts
+
+    delete require.cache[require.resolve('next/dist/telemetry/ci-info')]
+    delete require.cache[require.resolve('next/dist/next-server/server/config')]


Considering next is performing some logic at require()-time, this approach makes sense.

The only alternative I can think of would be to call getNextConfig() in a separate process. However, this would add some significant complexity. Also, this would JSON-serialize the Next.js configuration, which would not work if the config includes non-JSON values (can it?), unless we use serialization: 'advanced' but that option is not available in Node 10.

Two potential problems we might have in the future though are:

I am unsure whether this would work with ES modules since the current Node.js implementation does not allow removing cached URLs (it allows passing query parameters as cache key, but this would not work here), so this might require mixing CommonJS require() and ES modules import in order to keep using require.cache.

This relies on next internal file structure. The cache deletion would be a noop if they choose to rename those files. Some things we could do to fix this:

Run some automated tests confirming those files exist with the latest version of next

delete all the next entries in require.cache instead of those specific files

Unfortunately we're already relying on the internal layout as we're using unpublished exports to get the config in the first place

lindsaylevine · 2021-05-25T13:35:05Z

yes that's definitely coming up!!! @Mickael

…

On Tue, May 25, 2021 at 9:14 AM ehmicky ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In helpers/hasCorrectNextConfig.js <#343 (comment)> : > + /* eslint-disable fp/no-delete, node/no-unpublished-require */ + + // We emulate Vercel so that we can set target to serverless if needed + process.env.NOW_BUILDER = true + // If no valid target is set, we use an internal Next env var to force it + process.env.NEXT_PRIVATE_TARGET = 'serverless' + + // 🐉 We need Next to recalculate "isZeitNow" var so we can set the target, but it's + // set as an import side effect so we need to clear the require cache first. 🐲 + // https://github.com/vercel/next.js/blob/canary/packages/next/telemetry/ci-info.ts + + delete require.cache[require.resolve('next/dist/telemetry/ci-info')] + delete require.cache[require.resolve('next/dist/next-server/server/config')] + + // Clear memoized cache + getNextConfig.clear() This works. In the long run, we probably should consider removing memoization on getNextConfig() and passing its return value around function calls instead. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#343 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANVRQIWQHNFTCFCZQ2TKCKDTPOPFBANCNFSM45M7X4CQ> .

ehmicky · 2021-05-25T13:43:22Z

helpers/hasCorrectNextConfig.js

+    // We emulate Vercel so that we can set target to serverless if needed
+    process.env.NOW_BUILDER = true
+    // If no valid target is set, we use an internal Next env var to force it
+    process.env.NEXT_PRIVATE_TARGET = 'serverless'


I believe some automated tests might be using the same process, i.e. running one test would change process.env for other tests. We could probably fix this by either:

Spawning those tests in different processes

Loading next right away so it uses those environment variables, then deleting process.env.NOW_BUILDER and process.env.NEXT_PRIVATE_TARGET (or restoring them to their original value).

ehmicky · 2021-05-25T13:50:37Z

@ehmicky Can you think of a way for us to detect if the plugin is returning early when we're in tests? You mentioned here that we shouldn't return values in plugin methods.

I would personally suggest either:

Running a unit test instead of an integration test, i.e. importing the specific function to test instead of running the whole plugin
Testing a side effect produced by the plugin returning early.

ascorbic · 2021-05-25T14:55:17Z

I worked it out. I extracted the target updating code into a different funciton, so it wasn't combined with the checks which I then run before. In the tests I check whether process.env.NEXT_PRIVATE_TARGET has been set, which will only happen if the checks have passed.

lindsaylevine

👏 💯

lindsaylevine · 2021-05-25T15:41:42Z

helpers/verifyBuildTarget.js

+  /* eslint-enable fp/no-delete, node/no-unpublished-require */
+}
+
+module.exports = verifyBuildTarget


love this 🙏

lindsaylevine · 2021-05-25T15:41:52Z

helpers/verifyBuildTarget.js

+  console.log(
+    `The "target" config property must be one of "${acceptableTargets.join(
+      '", "',
+    )}". Building with "serverless" target.`,


lindsaylevine · 2021-05-25T15:42:15Z

index.js

-      }
-    }
+    // Populates the correct config if needed
+    await doesNotNeedPlugin({ netlifyConfig, packageJson, failBuild })

    // Because we memoize nextConfig, we need to do this after the write file


double ping on this one?

Suggested change

// Because we memoize nextConfig, we need to do this after the write file

lindsaylevine · 2021-05-25T15:43:17Z

test/index.js

+  delete require.cache[require.resolve('next/dist/telemetry/ci-info')]
+  delete require.cache[require.resolve('next/dist/next-server/server/config')]
+
+  getNextConfig.clear()


:O omg. we couldve done this the whole time...................

lindsaylevine · 2021-05-25T15:43:26Z

test/index.js

@@ -89,14 +85,15 @@ describe('preBuild()', () => {
  })

  test('run plugin if the app has next export in an unused script', async () => {
+    process.env.hi = 'ok'


lindsaylevine · 2021-05-25T15:43:40Z

test/index.js

-
-    expect(await pathExists('next.config.js')).toBeTruthy()
+    expect(process.env.NEXT_PRIVATE_TARGET).toBe('serverless')
+    // expect(await pathExists('next.config.js')).toBeTruthy()


Suggested change

// expect(await pathExists('next.config.js')).toBeTruthy()

lindsaylevine · 2021-05-25T15:43:50Z

test/index.js

@@ -133,7 +130,7 @@ describe('preBuild()', () => {
      utils,
    })

-    expect(await pathExists('next.config.js')).toBeFalsy()
+    expect(process.env.NEXT_PRIVATE_TARGET).toBeUndefined()


Co-authored-by: Michal Piechowiak <[email protected]>

fix: force serverless target

8dc7ca1

github-actions bot added the type: bug code to address defects in shipped code label May 24, 2021

ascorbic requested a review from lindsaylevine May 24, 2021 10:45

lindsaylevine reviewed May 25, 2021

View reviewed changes

fix: changes from review

5b71269

ehmicky reviewed May 25, 2021

View reviewed changes

ascorbic added 2 commits May 25, 2021 15:52

fix: extract target checks

a1ab7bf

fix: fix tests

630cb1a

ascorbic requested a review from lindsaylevine May 25, 2021 15:43

lindsaylevine reviewed May 25, 2021

View reviewed changes

fix: changes from review

09642bd

ascorbic requested a review from lindsaylevine May 25, 2021 15:58

lindsaylevine approved these changes May 25, 2021

View reviewed changes

ascorbic merged commit b43b49f into main May 25, 2021

ascorbic deleted the mk/force-serverless branch May 25, 2021 16:37

This was referenced Jun 4, 2021

v3.4.1 breaks netlify nextjs build #387

Closed

Revert "fix: force serverless target" #388

Merged

ascorbic mentioned this pull request Jun 11, 2021

fix: force serverless target #408

Merged

serhalp pushed a commit that referenced this pull request Apr 5, 2024

chore: fix scheduled e2e workflow (#343)

d9ea71e

Co-authored-by: Michal Piechowiak <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: force serverless target #343

fix: force serverless target #343

ascorbic commented May 24, 2021

lindsaylevine commented May 24, 2021

lindsaylevine left a comment

lindsaylevine May 25, 2021

ascorbic May 25, 2021

ehmicky May 25, 2021

lindsaylevine May 25, 2021

lindsaylevine May 25, 2021

lindsaylevine May 25, 2021

ascorbic May 25, 2021

ascorbic commented May 25, 2021

ascorbic commented May 25, 2021

lindsaylevine commented May 25, 2021

ehmicky May 25, 2021

ascorbic May 25, 2021

lindsaylevine commented May 25, 2021 via email

ehmicky May 25, 2021

ehmicky commented May 25, 2021

ascorbic commented May 25, 2021

lindsaylevine left a comment

lindsaylevine May 25, 2021

lindsaylevine May 25, 2021

lindsaylevine May 25, 2021

lindsaylevine May 25, 2021

lindsaylevine May 25, 2021

ascorbic May 25, 2021

lindsaylevine May 25, 2021

lindsaylevine May 25, 2021

fix: force serverless target #343

fix: force serverless target #343

Conversation

ascorbic commented May 24, 2021

lindsaylevine commented May 24, 2021

lindsaylevine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascorbic commented May 25, 2021

ascorbic commented May 25, 2021

lindsaylevine commented May 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindsaylevine commented May 25, 2021 via email

Choose a reason for hiding this comment

ehmicky commented May 25, 2021

ascorbic commented May 25, 2021

lindsaylevine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment